Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add terraform block to config when not already present #216

Merged
merged 23 commits into from
Nov 30, 2023

Conversation

bendbennett
Copy link
Contributor

@bendbennett bendbennett commented Nov 13, 2023

Closes: #215

Dependency: #228

Considerations/Questions

  • Currently, there is no handling to add required provider entries into the terraform block if TestStep.Config contains JSON.
    • Does this need implementing?
  • The entries in the terraform required providers block are not added prior to the initial call to wd.Init, or in testIDRefresh()
    • Interested to hear comments on whether this is viewed as ok.
  • The providerConfigTestCase() removes entries from testCase.Providers, when an entry with the same key is found in testCase.ProviderFactories.
    • This was required to avoid a panic generated by the presence of two entries with the same key in the terraform required providers block when running TestTest_TestCase_Providers (i.e., TestTest_TestCase_Providers results in test appearing as a key in both testCase.Providers, and testCase.ProviderFactories.

@bendbennett bendbennett added the enhancement New feature or request label Nov 13, 2023
@bendbennett bendbennett requested a review from a team as a code owner November 13, 2023 17:16
@bflad bflad added this to the v1.6.0 milestone Nov 15, 2023
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that we could potentially negatively affect working tests after this feature is added, I would suggest we go ahead and also include an enhancement changelog for this, e.g.

helper/resource: Automatically add `required_providers` configuration to `TestStep.Config` configuration when necessary to support future provider functionality testing

if !c.IsJSON() && !c.HasTerraformBlock() {
str := strings.TrimLeft(string(c), "\n")

return fmt.Sprintf("terraform {}\n\n%s", str)
Copy link
Contributor

@bflad bflad Nov 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this to work as expected, the terraform configuration in this case must include a correct required_providers definition to match the provider(s) being tested and included in the configuration, e.g.

terraform {
  required_providers {
    NAME = {
      source = "ADDRESS"
    }
  }
}

This is what future Terraform provider functionality will require in the configuration so Terraform explicitly knows which provider is being referenced in a configuration (rather than implicitly guessing any missing provider declaration is in the hashicorp namespace). Here's some internal background about this problem: https://github.com/hashicorp/terraform-proposals/issues/66

There is somewhat of an example of this in (TestStep).providerConfig() but its only wired to the ExternalProviders information. This implementation needs to account for a lot more, unfortunately:

I believe the NAME values must contain all the map keys of all the following defined on the TestCase/TestStep:

  • Providers
  • ProviderFactories
  • ExternalProviders
  • ProtoV5ProviderFactories
  • ProtoV6ProviderFactories

(† We may not necessarily need to include Providers/ProviderFactories as terraform-plugin-sdk/v2 providers will never receive the future enhancements needing the required provider configuration, however I think to prevent potential bugs where Providers/ProviderFactories and ExternalProviders is being used, we probably should just properly do it for everything.)

And the ADDRESS value may also need to depend on things like the TF_ACC_PROVIDER_NAMESPACE environment variable. We might need to introduce some additional testing with non-hashicorp namespace providers and the ProtoV{5,6}ProviderFactories fields to verify whether that value must be consulted or not.

Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I think this is what we want! To answer some of your questions:

Currently, there is no handling to add required provider entries into the terraform block if TestStep.Config contains JSON.
Does this need implementing?

I'd say no for now -- we have treated JSON configuration as a "you know what you're doing", similar to the new configuration file/directory handling.

The entries in the terraform required providers block are not added prior to the initial call to wd.Init, or in testIDRefresh()

I think this is okay. Init only needs external providers (or whatever it currently gets). ID refresh is a very legacy testing feature that doesn't need to be touched. It honestly should be deprecated, but I forget why that hasn't already been done other than maybe previously thinking all the next major version deprecations would be done at once.


At some point we'll clean up all those gnarly helper/resource function signatures. 😬 😅

Comment on lines 24 to 27

requiredProviderBlocks.WriteString(fmt.Sprintf(" %s = {\n", name))

requiredProviderBlocks.WriteString(" }\n")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can omit this handling here (at least for now) -- its odd to have empty required_provider configuration and honestly not sure if Terraform will always treat that as valid. 👍

)

const tfBlockMinReqTFVersion = "1.6.0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we discussed this out of band, but it might be good to set this to 1.0.0 with the reasoning that we'd eventually remove this version check entirely in the terraform-plugin-testing major version we drop protocol version 5 support. Another potentially good reason to do this now is that it won't potentially delay feedback for folks who may be already be testing on 1.x and upgrade their terraform-plugin-testing.

If you could also drop a quick code comment here about why this version check exists at all, that would be fantastic!

@@ -0,0 +1,6 @@
kind: ENHANCEMENTS
body: 'helper/resource: Automatically add `required_providers` configuration to `TestStep.Config`
configuration when using Terraform 1.6.* and above'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good consideration callout in the PR description about JSON, since we currently treat JSON as "you know what you're doing" and our future functionality testing documentation will always show Terraform configuration language, I think just putting a quick "Terraform language" before the word "configuration" here will suffice. Also if we do update the check to 1.0, that will need to be updated here as well. 😄

@bendbennett bendbennett merged commit 222d08e into main Nov 30, 2023
26 checks passed
@bendbennett bendbennett deleted the bendbennett/issues-215 branch November 30, 2023 09:33
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add terraform block to config if not already present
2 participants